Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CO-389] Write properties for cardano-sl-x509 #3620

Merged
merged 3 commits into from
Sep 21, 2018
Merged

[CO-389] Write properties for cardano-sl-x509 #3620

merged 3 commits into from
Sep 21, 2018

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Sep 18, 2018

Description

As per comment in #3610 --> #3610 (comment)

Linked issue

[CO-379]

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • [ ] CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

@KtorZ
[CO-389] Write property for x509 cert generation
560df73 
This make sure that for any configuration, we actually generate valid certificates. Note that the property
doesn't pass for we provide AltDNS that are IP addresses and this turns out to be invalid. The underlying v3
extensions only supports DNS names and we should make sure we properly throw when given an IP address
@KtorZ KtorZ requested a review from erikd September 18, 2018 06:18
erikd
erikd approved these changes Sep 18, 2018
View reviewed changes
x509/test/Test/Cardano/X509/Configuration/Arbitrary.hs
<*> arbitraryBasicString
<*> oneof [pure Nothing, Just <$> arbitraryBasicString]

shrink _ = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you have implemented this in QuickCheck and there is no shrink. If this fails, its going to be a pain in the neck.

We'll keep this for now, but I really recommend that you have a look at HedgeHog.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not really right? There are shrinkers defined when they make sense. Here, there's just no way to shrink a DirConfiguration. Nevertheless, even without shrinking, a DirConfiguration don't get really unmanageable 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually why I have also explictely defined it as [], to make it clear that I haven't forgotten to define a shrink method for that type but that is just that this is the only sensible choice :)

And so far, failures render nicely 😛 (I got a few during development)

disassembler
disassembler suggested changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@disassembler disassembler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add this to release.nix so hydra reports checks for it since it's no longer part of tools.

x509/src/Cardano/X509/Configuration.hs Outdated
parseAltName :: MonadThrow m => String -> m AltName
parseAltName name =
case IP.decode (T.pack name) of
Just _ -> throwM (ErrInvalidAltName "IP was given as an AltName but it should be a DNS name")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break deadalus. Please leave the IP SAN parsing in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Good catch @Disasm
But IP SAN aren't recognized as a valid alternative names by the x509 library we use. I can perhaps tweak the hook which validates the name...

Does Daedalus uses 127.0.0.1 to contact the server instead of localhost?

x509/test/Test/Cardano/X509/Configuration/Arbitrary.hs Outdated
instance Arbitrary (Invalid AltNames) where
arbitrary =
fmap (Invalid . mkAltNames) $ listOf1 $ frequency
[ (80, pure "localhost")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean localhost is an invalid altName as well now? That breaks wallet connectScripts if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The Invalid is just biaised to sometimes generate invalid configurations such that we can use it to trigger multiple sort of errors.

@KtorZ KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch from b9b3894 to ea5bc21 Compare September 19, 2018 06:11
@KtorZ
Copy link
Contributor Author

KtorZ commented Sep 19, 2018

@disassembler

Okay, I've updated the implementation to:

  • Accept IPs as SAN (using the IPv4 and IPv6 encoding / decoding functions)
  • Properly validate IP SAN and DNS SAN. The former one just does a plain equality check and the latter relies on the default validation hook from x509-validation (which validates DNS SAN or fails)
  • Updated properties to reflect these changes
  • Have a new property which checks that unknown ServiceID (i.e. not defined as SAN) are actually rejected, for server certificates (we don't check the name in case of client certificates because it doesn't make sense).

Let me know 👍

Also: I am not sure whether I should add something to release.nix or anything. Note that this package is going to be used by cardano-sl-wallet-new (PR incoming) so it will get built anyway. Would tests run as part of the build automatically?

@KtorZ KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch 2 times, most recently from e705dfd to 46afb5d Compare September 21, 2018 11:30
@KtorZ
Copy link
Contributor Author

KtorZ commented Sep 21, 2018

@disassembler
So, while using this on the "demo cluster" story, I noticed an issue with our TLS Manager with IP SANs. We basically need to have our custom name validator here as well, so I've added it:

https://github.com/input-output-hk/cardano-sl/pull/3620/files#diff-a879fd32cda407f37cf36a9459d628e8

@KtorZ
[CO-389] Fix x509 generation: throw when provided invalid config
88dc613 
e.g.
  - given a non-positive expiry days
  - given an unknown ServiceID
@KtorZ KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch 2 times, most recently from 003ec4a to 91ef67e Compare September 21, 2018 12:01
@KtorZ KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch from 91ef67e to cac524c Compare September 21, 2018 13:17
@KtorZ KtorZ merged commit b4a083d into develop Sep 21, 2018
@KtorZ KtorZ deleted the KtorZ/CO-389/cardano-sl-x509-as-library branch September 21, 2018 22:10
KtorZ added a commit that referenced this pull request Nov 9, 2018
@KtorZ
Merge pull request #3620 from input-output-hk/KtorZ/CO-389/cardano-sl…
9202fc8 
…-x509-as-library

[CO-389] Write properties for cardano-sl-x509
KtorZ added a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018
@KtorZ
Merge pull request input-output-hk/cardano-sl#3620 from input-output-…
3ab6d39 
…hk/KtorZ/CO-389/cardano-sl-x509-as-library

[CO-389] Write properties for cardano-sl-x509
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikd erikd erikd approved these changes

@disassembler disassembler disassembler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KtorZ @erikd @disassembler